Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add csv endpoint wrapper #38

Merged
merged 10 commits into from
Oct 21, 2024
Merged

Add csv endpoint wrapper #38

merged 10 commits into from
Oct 21, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Oct 18, 2024

Brief overview of changes

Add a wrapper function for getting a CSV version of an API data set from EES.

Why are these changes being made?

There's a GET endpoint for datasets where you can specify CSV as the response type. There's no querying or even dataset version options on this endpoint yet so it's literally just a download of the latest version.

Detailed description of changes

Updated api_url() to have a new response_type argument where you can switch between JSON or CSV, expecting we can build on this more if the CSV response type gets interest and requests for more features.

I figured api_url() made the most sense for these changes, though happy to be told this jars with a different vision you have @rmbielby - aware you've put a lot of thought into the modularising of the code in the package.

Additional information for reviewers

Couple of small bits of tidy up.

  1. Set package to depend on R version 3.5.0 or higher in DESCRIPTION

Did this as there were messages appearing in devtools::check() about this related to the test data we have in the package.

Debated adding an on load warning for users like this in a zzz.R file:

.onLoad <- function(libname, pkgname) {
  if (getRversion() < "3.5.0") {
    stop("This package requires R version 3.5.0 or later.")
  }
}

But decided against as overkill, as it probably won't affect most users.

  1. readr as a suggested import

httr uses readr behind the scenes, not including it was causing errors when running tests / examples, but including as a full import was giving a warning as we never call readr directly. This seemed to work as a solution that appeased the R package gods.

  1. Added an internal toggle_message() function

Now, I know we have this in dfeR, though I wanted to avoid having all of dfeR and it's dependencies and dependencies on this.

Felt like a small enough function to copy over and stick in a utils.R file for us to use ourselves to control verbosity. Expect we'd take a similar approach with other packages too.

  1. upped cyclocomp limits for lintr

We were exceeding on a couple of functions. I think we're mostly safe with what we're doing for now and are modularising a lot already so a higher limit of 25 seemed more suitable and hushed lintr for the time being.

  1. I ran usethis::use_tidy_description()

It moved some stuff about in the DESCRIPTION file.

Issue ticket number/s and link

Resolves #36.

@cjrace cjrace added the enhancement Improvement to existing feature label Oct 18, 2024
@cjrace cjrace added this to the Ready for user testing milestone Oct 18, 2024
R/download_dataset.R Dismissed Show dismissed Hide dismissed
R/download_dataset.R Dismissed Show dismissed Hide dismissed
@cjrace cjrace added new feature New feature or request and removed enhancement Improvement to existing feature labels Oct 18, 2024
@cjrace cjrace requested a review from rmbielby October 18, 2024 12:03
Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a quick scan through and looks good. Was a bit thrown when you hadn't just added a specific "get-csv" endpoint to api_url, which would have been my first thought, but think this way makes sense.

Main thought is handling the additional potential for redundant parameters in api_url(). I flag a warning if the time_periods, indicators, etc parameters are set when someone's running an endpoint that doesn't use them, e.g. "get-meta". At the very least I think that flag should trigger for this scenario as well. Plus response_format = CSV doesn't apply to any other endpoint, so should probably be a warning if someone sets it to CSV for any of those requests.

I'm a little wary that if this is the only endpoint that's ever going to be able to deliver a csv, then response_format is a bit redundant and just "get-csv" as an endpoint might make more sense and make the code simpler in terms of having one less thing that doesn't do anything across all the other endpoints.

I guess on the above "expecting we can build on this more if the CSV response type gets interest and requests for more features" is carrying the weight here, so if we think that's likely, I guess just adding in some warnings would be helpful.

@cjrace
Copy link
Contributor Author

cjrace commented Oct 18, 2024

Interesting to see where your brain instinctively jumped to. I was in two minds when I approached it and this was enough to make me go back and review it a bit more in the API docs.

While in my head this might in the long run just be an option on the /query endpoints for response type, currently in the API itself it has been implemented as a separate /csv endpoint so for now I've switched over to that, treating it as a get-csv endpoint in the code, and we can worry about response_type at a later point once we know the future shape of the API better.

Have also made sure the warnings appear for unecessary arguments and added a quick test for it too.

R/api_url.R Outdated Show resolved Hide resolved
@cjrace cjrace merged commit 6c6d85c into main Oct 21, 2024
9 checks passed
@cjrace cjrace deleted the feature/add-csv-endpoint-wrapper branch October 21, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CSV endpoint wrapper
2 participants